Implement Memory1 (RULE-8-7-1)
#967
Conversation
…terminator, remove file pointer cases 1. Add headers, Adding missing headers: For obvious reasons. 2. Remove cases without null terminator: Both clang and g++ do not permit strings to be allocated that are declared to be shorter than the actual initializing expression. Since this is a C++ rule, we rule them out. 3. File pointer manipulation functions (e.g. fgets): Not required by the rule.
…ub/codeql-coding-standards into jeongsoolee09/MISRA-C++-2023-Memory
Too many of the negative offsets alerts were false positives. We leave it for future work.
There was a problem hiding this comment.
Pull request overview
This PR implements MISRA C++ 2023 Rule 8-7-1 (Memory1 package) which validates pointer arithmetic to ensure pointers don't exceed array bounds. The PR updates the rules.csv to organize memory-related rules into separate packages (Memory1-Memory6) and adds two queries for detecting invalid pointer arithmetic.
Changes:
- Implements two queries for RULE-8-7-1: PointerArithmeticFormsAnInvalidPointer (path-problem) and PointerArgumentToCstringFunctionIsInvalid (problem)
- Adds a comprehensive OutOfBounds.qll module (1358 lines) for buffer overflow analysis
- Updates rules.csv to assign memory-related rules to Memory1-Memory6 packages
- Adds Memory1.json rule package description file and Memory1.qll exclusions file
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Updates package names from "Memory" to "Memory1" through "Memory6" for memory-related rules |
| rule_packages/cpp/Memory1.json | Adds package description for Memory1 with metadata for both queries |
| cpp/misra/src/rules/RULE-8-7-1/PointerArithmeticFormsAnInvalidPointer.ql | Implements path-problem query for detecting invalid pointer arithmetic |
| cpp/misra/src/rules/RULE-8-7-1/PointerArgumentToCstringFunctionIsInvalid.ql | Implements problem query for invalid cstring function arguments |
| cpp/misra/test/rules/RULE-8-7-1/test.cpp | Comprehensive test file with 519 lines covering various pointer arithmetic scenarios |
| cpp/misra/test/rules/RULE-8-7-1/*.expected | Expected results files for both queries |
| cpp/misra/test/rules/RULE-8-7-1/*.qlref | Query reference files |
| cpp/common/src/codingstandards/cpp/OutOfBounds.qll | New module providing buffer overflow analysis infrastructure |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Memory1.qll | Auto-generated exclusions file for Memory1 queries |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Updates to include Memory1 query metadata |
| cpp/common/test/includes/standard-library/*.h | Adds missing C standard library function declarations |
| cpp/common/test/includes/standard-library/cstdlib | Adds using declarations for malloc, calloc, realloc |
Comments suppressed due to low confidence (1)
cpp/misra/src/rules/RULE-8-7-1/PointerArithmeticFormsAnInvalidPointer.ql:171
- Similar to the PointerAddExpr case, this predicate uses getAnOperand() which could match either operand. For PointerSubExpr, you need to specifically get the right operand (the value being subtracted). The current implementation could incorrectly negate the pointer operand instead of the offset value.
exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() |
result = -pointerSubtraction.getAnOperand().getValue().toInt()
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import codingstandards.cpp.exclusions.c.RuleMetadata | ||
|
|
||
| from | ||
| OOB::BufferAccessLibraryFunctionCall fc, string message, Expr bufferArg, string bufferArgStr, | ||
| Expr sizeOrOtherBufferArg, string otherStr | ||
| where | ||
| not isExcluded(fc, OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery()) and |
There was a problem hiding this comment.
This query imports from codingstandards.cpp.exclusions.c.RuleMetadata but the OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery is defined for C, not C++. This is a MISRA C++ rule (RULE-8-7-1), so it should either use the C++ exclusions system or verify that using the C exclusions is intentional. This could cause issues with the exclusion system not properly recognizing this as a C++ query.
| } | ||
|
|
||
| /** | ||
| * The |
There was a problem hiding this comment.
Incomplete documentation: The class documentation starts with "The" but is incomplete. Please complete the documentation explaining what this class represents, such as "A heap allocation function call that is cast to a specific pointer type."
| * The | |
| * A heap allocation function call that is cast to a specific pointer type. |
| result = pointerAddition.getAnOperand().getValue().toInt() // TODO: only get the number being added | ||
| ) | ||
| or | ||
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | ||
| result = -pointerSubtraction.getAnOperand().getValue().toInt() |
There was a problem hiding this comment.
The getOffset predicate has a TODO comment indicating incomplete implementation. For PointerAddExpr, the current logic uses getAnOperand() which could match either the pointer or the integer operand. This should specifically get the right operand (the offset value) to avoid incorrect results. Consider using getRightOperand() or a similar specific accessor to ensure the correct operand is used.
| result = pointerAddition.getAnOperand().getValue().toInt() // TODO: only get the number being added | |
| ) | |
| or | |
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | |
| result = -pointerSubtraction.getAnOperand().getValue().toInt() | |
| result = pointerAddition.getRightOperand().getValue().toInt() | |
| ) | |
| or | |
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | |
| result = -pointerSubtraction.getRightOperand().getValue().toInt() |
Description
Implement Memory1 (
RULE-8-7-1) and add rule package description files for the rest of the rules (Memory2-Memory6).Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-8-7-1Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.